Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clear cookies when switching accounts #8490

Merged

Conversation

ClementineAccount
Copy link
Contributor

This closes #8005.

This follows the suggestions proposed by Mek in internetarchive/infogami#215 (comment)
by creating handlers to call a new function that clears the cookies "pd" and "sfw"

Technical

This implementation solves the issue mentioned by clearing the cookies using a handler to infogami, overriding infogami logout function. It also does this when 'logging in' as another user from the admin panel of admin/people

Testing

(The video below under Screenshot also shows this step)

  1. Login as an admin and observe the 'pd' cookie be created through the inspector
  2. Logout as an admin and observe the 'pd' cookie has been cleared.
  3. Login as admin again and observe that the pd cookie has been created again.
  4. Login as another user using admin/people and observe that the pd cookie has been cleared.

Screenshot

I recorded this video showcasing the 'pd' cookie being cleared when the hotfix is applied. Prior to the hotfix application, the pd cookie will never be cleared.

2023-11-03.20-31-15.mp4

Stakeholders

@mekarpeles proposed the fix concept and suggested this issue to me as a 'Good First Issue' so he would likely want to review my approach prior to a merge.

@ClementineAccount
Copy link
Contributor Author

I failed one of the CI/CD tests by accidentally importing infogami_logout twice. I have since fixed that but need to run the workflow again.

@mekarpeles
Copy link
Member

provisional LGTM, great job -- will state on testing and give it a try

@mekarpeles mekarpeles self-assigned this Nov 6, 2023
@mekarpeles mekarpeles added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Nov 6, 2023
@cdrini
Copy link
Collaborator

cdrini commented Nov 21, 2023

Taking off testing to test #8550 .

@cdrini cdrini removed the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Nov 21, 2023
@mekarpeles mekarpeles assigned jimchamp and unassigned mekarpeles Nov 30, 2023
@jimchamp jimchamp changed the title Hotfix for #8005 by clearing cookies at correct time Clear cookies when switching accounts Dec 5, 2023
@mekarpeles mekarpeles added the Priority: 2 Important, as time permits. [managed] label Dec 11, 2023
@jimchamp jimchamp added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Dec 12, 2023
Copy link
Collaborator

@jimchamp jimchamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks to be working as expected. Thanks, @ClementineAccount!

@jimchamp jimchamp merged commit f51dab7 into internetarchive:master Dec 12, 2023
3 checks passed
@jimchamp jimchamp removed the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 2 Important, as time permits. [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Log in as user" feature does not reset printdisabled cookie
4 participants